fix: correctness bugs in flagged-storage, recovery, and docs#516
fix: correctness bugs in flagged-storage, recovery, and docs#516ndycode wants to merge 1 commit into
Conversation
Found via a parallel multi-agent review sweep of the lib/ tree. Only high-confidence defects with observable wrong behavior were fixed; several reviewer findings were verified as false positives or by-design and deliberately left alone (see PR notes). - flagged-storage: the lastSwitchReason/cooldownReason type guards in normalizeFlaggedStorage dropped two valid enum members. "manual" (lastSwitchReason) and "server-error" (cooldownReason) are part of the type, the Zod schema, and are produced at runtime, but the guards omitted them — so on any load/normalize/save round-trip those values were silently coerced to undefined, erasing switch/cooldown provenance from flagged records. Added both to the guards. (+2 regression tests) - recovery/findEmptyMessageByIndex: add the `role === "assistant"` guard that its sibling findMessageByIndexNeedingThinking already has, so the index-offset fallback can't select an unrelated user message to repair. (Currently export-only with no live caller; hardening to prevent misuse.) - rotation: HybridSelectionConfig.freshnessWeight JSDoc said "default: 0.1" while DEFAULT_HYBRID_SELECTION_CONFIG uses 2.0; corrected the doc. - README: fix the "Current prerelease" link, which still pointed at v2.3.0-beta.0 after the v2.3.0-beta.1 release (the doc exists). This is what test/documentation.test.ts (release-history) asserts. - Added test/fs-retry.test.ts covering shouldRetryFileOperation across the full FILE_RETRY_CODES set and rejection of non-transient / code-less errors. Verified: build, typecheck, lint, full vitest suite (4433 passed, 0 failed, 3 skipped). npm audit unaffected.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🧰 Additional context used📓 Path-based instructions (11)lib/**/*.ts📄 CodeRabbit inference engine (lib/AGENTS.md)
Files:
lib/storage/**/*.ts📄 CodeRabbit inference engine (lib/AGENTS.md)
Files:
**/*.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,js}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,mjs,cjs}📄 CodeRabbit inference engine (README.md)
Files:
lib/**⚙️ CodeRabbit configuration file
Files:
**⚙️ CodeRabbit configuration file
Files:
test/**/*.test.ts📄 CodeRabbit inference engine (test/AGENTS.md)
Files:
test/**/*.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
test/**⚙️ CodeRabbit configuration file
Files:
lib/rotation.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (11)📓 Common learnings📚 Learning: 2026-06-07T09:19:57.566ZApplied to files:
📚 Learning: 2026-06-07T09:19:57.566ZApplied to files:
📚 Learning: 2026-06-07T09:19:57.566ZApplied to files:
📚 Learning: 2026-06-04T06:14:18.093ZApplied to files:
📚 Learning: 2026-06-04T06:14:24.975ZApplied to files:
📚 Learning: 2026-06-07T09:19:57.566ZApplied to files:
📚 Learning: 2026-06-07T20:01:36.351ZApplied to files:
📚 Learning: 2026-06-07T20:01:36.351ZApplied to files:
📚 Learning: 2026-06-07T20:01:36.351ZApplied to files:
📚 Learning: 2026-06-07T20:01:36.351ZApplied to files:
🔇 Additional comments (4)
📝 WalkthroughSummaryThis PR fixes two correctness bugs and one documentation issue with major severity in data persistence and medium severity in message filtering. The flagged-storage fix addresses a data normalization bug where two valid enum members ( Additional changes:
Verification: All builds (build, typecheck, lint) passed; test suite: 4433 passed, 0 failed, 3 skipped (includes 5 new tests). Walkthroughversion bump to beta.1, recovery storage filters messages by assistant role, flagged storage accepts server-error cooldown reasons with test coverage, fs-retry tests validate transient vs. permanent errno handling, and rotation config docs update freshnessWeight default. ChangesRelease Notes and Storage Validation Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
test/flagged-storage.test.tsOops! Something went wrong! :( ESLint: 10.0.0 Error: The 'jiti' library is required for loading TypeScript configuration files. Make sure to install it. test/fs-retry.test.tsOops! Something went wrong! :( ESLint: 10.0.0 Error: The 'jiti' library is required for loading TypeScript configuration files. Make sure to install it. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
A parallel multi-agent review sweep over the
lib/tree (rotation/proxy, accounts/storage/recovery, request/auth/refresh, config/manager/usage). I fixed only high-confidence defects with observable wrong behavior, added regression tests, and deliberately left several findings alone after verifying they were false positives or by-design (documented below).Fixes
1. Flagged-storage silently drops valid enum values (data loss) —
lib/storage/flagged-storage.tsnormalizeFlaggedStorage's type guards omitted two members that are part of the type, the Zod schema, and produced at runtime:lastSwitchReason: "manual"(set by the CLIswitchcommand)cooldownReason: "server-error"(produced byfailure-policy.ts/runtime-rotation-proxy.ts)On any load → normalize → save round-trip, these were coerced to
undefined, erasing switch/cooldown provenance from flagged records. +2 regression tests.2. Recovery message-repair could target the wrong message —
lib/recovery/storage.tsfindEmptyMessageByIndexprobedtargetIndex, -1, -2and returned the first empty message without a role check, unlike its siblingfindMessageByIndexNeedingThinkingwhich requiresrole === "assistant". Added the matching guard so it can't select an unrelated user message. (Export-only/no live caller today — hardening against misuse.)3. Misleading JSDoc default —
lib/rotation.tsHybridSelectionConfig.freshnessWeightdoc saiddefault: 0.1; the actual default is2.0. A consumer wiring a custom config from the doc would under-weight freshness 20×. Doc corrected.4. README prerelease link drift —
README.md"Current prerelease" still pointed at
v2.3.0-beta.0after thev2.3.0-beta.1release. Fixed (asserted bytest/documentation.test.ts).5. New test coverage —
test/fs-retry.test.tsCovers
shouldRetryFileOperationacross the fullFILE_RETRY_CODESset and rejection of non-transient / code-less errors (this predicate had no direct test).Findings deliberately NOT changed (verified, not bugs)
renameTempToPathonly retries EPERM/EBUSY (lib/storage.ts): a reviewer flagged this as inconsistent with the module's broader retry set. It is by-design and explicitly tested —storage.test.ts > "throws immediately on non-EPERM/EBUSY errors"assertsEACCESon the final atomic commit-rename fails fast (a real permissions problem, not a transient lock). I made this change, it broke 7 tests, and I reverted it.lastGlobalSwitchAtstamped on every successful request (runtime-rotation-proxy.ts): plausibly intentional "drain-until-exhausted" stickiness, consistent with the recent sequential drain-first feature. Changing it alters core rotation semantics — needs maintainer intent, not a blind fix.activeIndexre-clamp (import-export.ts): "skipped vs updated" count semantics are a product judgment encoded in existing tests; the activeIndex concern is masked by load-timenormalizeAccountStorage. No observable defect.Verification
npm run build✅npm run typecheck✅npm run lint✅npm test✅ — 4433 passed, 0 failed, 3 skipped (5 new tests vs. baseline)Note
Independent of the hono security bump in #515 — that PR can merge separately.
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr fixes three correctness bugs found in a multi-agent sweep: silent data loss in
normalizeFlaggedStorage(two missing enum members caused validlastSwitchReasonandcooldownReasonvalues to be coerced toundefinedon every save round-trip), a wrong-message selection risk infindEmptyMessageByIndex(no role guard, unlike its sibling), and a 20x wrongfreshnessWeightdefault in the jsdoc.\"manual\"toisSwitchReasonand\"server-error\"toisCooldownReason, aligning both guards with the fullCooldownReasonunion andAccountMetadataV3type; two regression tests included.findEmptyMessageByIndexnow requiresrole === \"assistant\"before returning a message id, preventing accidental repair of a user message; no live caller today but no test covers the new branch.freshnessWeight(0.1 to 2), bumps readme prerelease link, and adds first direct tests forshouldRetryFileOperationacross the fullFILE_RETRY_CODESset.Confidence Score: 4/5
safe to merge — all three code changes are narrow, well-scoped fixes with no altered control flow on production-active paths
the flagged-storage and rotation changes are clearly correct and well-tested. the recovery guard is sound but lacks a test for the new branch, and the fs-retry test hardcodes codes instead of iterating the exported set — both are minor coverage gaps that would be cheap to fill.
lib/recovery/storage.ts and test/fs-retry.test.ts would benefit from a second look on test completeness
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[load flagged-storage JSON] --> B[normalizeFlaggedStorage] B --> C{isSwitchReason?} C -->|rate-limit / initial / rotation / best / restore / manual| D[preserve lastSwitchReason] C -->|unknown value| E[coerce to undefined] B --> F{isCooldownReason?} F -->|auth-failure / network-error / server-error / rate-limit| G[preserve cooldownReason] F -->|unknown value| H[coerce to undefined] D & G --> I[save normalized record] J[findEmptyMessageByIndex] --> K[probe targetIndex -1 -2] K --> L{role === assistant?} L -->|no| M[skip not a repair target] L -->|yes| N{messageHasContent?} N -->|empty| O[return message id] N -->|has content| P[continue probe]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: correctness bugs in flagged-storage..." | Re-trigger Greptile